Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a performance issue where MoveWidgetCommand was being broadcast over the network every frame during widget movement in multiplayer mode, causing network flooding. The solution adds conditional broadcasting logic to only send the command when it's finalized.
- Added a public
IsFinalproperty to MoveWidgetCommand to expose the internal final state - Modified PhotonManager to only broadcast MoveWidgetCommand when
IsFinalis true - Added clarifying comments to the command merging logic in SketchMemoryScript
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Assets/Scripts/Commands/MoveWidgetCommand.cs | Exposes the final state through a public IsFinal property |
| Assets/Scripts/Multiplayer/Photon/PhotonManager.cs | Adds conditional logic to only broadcast MoveWidgetCommand when final |
| Assets/Scripts/SketchMemoryScript.cs | Adds clarifying comments to existing command merging logic |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| case MoveWidgetCommand: | ||
| var cmd = command as MoveWidgetCommand; | ||
| if (cmd.IsFinal) | ||
| { | ||
| success &= CommandBase(command); | ||
| } |
There was a problem hiding this comment.
When cmd.IsFinal is false, the method continues without setting success, but the return value of ProcessCommand depends on success. This could lead to incorrect return values when non-final MoveWidgetCommands are processed. Consider explicitly setting success = true in the else case or restructuring the logic to handle this scenario properly.
| case MoveWidgetCommand: | |
| var cmd = command as MoveWidgetCommand; | |
| if (cmd.IsFinal) | |
| { | |
| success &= CommandBase(command); | |
| } | |
| } | |
| else | |
| { | |
| success = true; | |
| } |
2 participants
Problem - some commands fire every frame and flood the network in multiplayer.
How to reproduce: Before joining a room, switch to advanced mode, add an image then switch back to beginner mode and create/join a room. Moving the widget will send the "move widget" RPC every single frame.
Cause: The current code assumes that adding a command to the undo stack is a sign that it should be sent over the network. However some command subclasses work as follows - each command is executed and added to the stack. The next command looks at the previous command and decides if it can merge with it. In the case of MoveWidgetCommand this is true until m_IsFinal is set to true (usually when the widget is released).
The original PhotonManager code has a switch/case that has a default fall-through for commands that aren't handled explicitly. I haven't removed this in this PR as I would need to figure out all possible commands that we currently want to handle and add a case statement for them all. However - I think this is probably a better approach as we shouldn't be broadcasting commands without carefully looking at their behaviour.
This PR does hopefully fix the specific MoveWidgetCommand case. Currently not tested but I'm posting it to start the discussion and get a couple of eyeballs on it.